Conversation
| """Returns a list of all member values.""" | ||
| return [member.value for member in cls] | ||
|
|
||
| def __eq__(self, other: Any) -> bool: |
There was a problem hiding this comment.
this is a very vague eq, but I think it makes sense, let me know if you think otherwise though @allenporter
|
Okay this one finally has tests and is ready for review @allenporter |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the status handling system to make it dynamic based on device features rather than static model-specific subclasses. The StatusTrait now extends StatusV2 instead of model-specific Status classes and dynamically determines available modes (vacuum/fan, water/mop, and cleaning routes) based on device features and region.
Changes:
- Replaced model-specific Status subclasses with a unified StatusV2 approach using dynamic mode resolution
- Added new vacuum modes (CARPET, OFF_RAISE_MAIN_BRUSH) and water modes (MIN, MAX)
- Enhanced RoborockModeEnum with custom equality and hashing to support comparisons with strings and integers
- Added region parameter throughout the device initialization chain to support region-specific cleaning routes
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| roborock/devices/traits/v1/status.py | Complete refactor of StatusTrait to use StatusV2 with dynamic mode lookups instead of static typed enums |
| roborock/data/v1/v1_containers.py | Added new StatusV2 class with int-based fields instead of typed enums, marked old Status as deprecated |
| roborock/data/code_mappings.py | Added eq and hash methods to RoborockModeEnum for flexible comparisons |
| roborock/data/v1/v1_clean_modes.py | Added new CARPET and OFF_RAISE_MAIN_BRUSH vacuum modes, MIN and MAX water modes |
| roborock/devices/traits/v1/init.py | Reordered initialization to create device_features before status, added region parameter, added callable check |
| roborock/devices/device_manager.py | Passed region parameter to v1.create |
| tests/devices/traits/v1/test_status.py | Added comprehensive tests for StatusTrait refresh operations |
| tests/data/v1/test_v1_containers.py | Added StatusV2 tests (though incomplete coverage) |
| tests/mock_data.py | Added UTF-8 encoding to file operations and dss field to mock data |
| tests/devices/traits/v1/fixtures.py | Added region parameter to device fixture |
| tests/devices/test_v1_device.py | Added region parameter to device fixture |
| tests/e2e/snapshots/test_device_manager.ambr | Updated snapshot with new encrypted response data |
| tests/devices/snapshots/test_v1_device.ambr | Updated snapshot reflecting new StatusTrait structure |
| tests/devices/snapshots/test_device_manager.ambr | Updated snapshot with dss field addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __eq__(self, other: Any) -> bool: | ||
| if isinstance(other, str): | ||
| return self.value == other or self.name == other | ||
| if isinstance(other, int): | ||
| return self.code == other | ||
| return super().__eq__(other) | ||
|
|
||
| def __hash__(self) -> int: | ||
| return hash((self.code, self._value_)) | ||
|
|
||
|
|
There was a problem hiding this comment.
The custom eq method allows comparison with strings and integers (e.g., VacuumModes.QUIET == "quiet" or VacuumModes.QUIET == 101), but the hash method doesn't maintain the hash/equality contract. Objects that compare equal must have the same hash value. This means VacuumModes.QUIET == "quiet" is True, but hash(VacuumModes.QUIET) != hash("quiet"), which can cause issues when using enum values in sets or as dictionary keys alongside strings or integers. Consider removing the eq override or documenting that these enums should not be mixed with raw strings/ints in hash-based collections.
| def __eq__(self, other: Any) -> bool: | |
| if isinstance(other, str): | |
| return self.value == other or self.name == other | |
| if isinstance(other, int): | |
| return self.code == other | |
| return super().__eq__(other) | |
| def __hash__(self) -> int: | |
| return hash((self.code, self._value_)) |
| def test_status_v2() -> None: | ||
| """Test that StatusV2 can be created from a dictionary.""" | ||
| s = StatusV2.from_dict(STATUS) | ||
| assert s.msg_ver == 2 | ||
| assert s.msg_seq == 458 | ||
| assert s.state == RoborockStateCode.charging | ||
| assert s.battery == 100 | ||
| assert s.clean_time == 1176 | ||
| assert s.clean_area == 20965000 | ||
| assert s.square_meter_clean_area == 21.0 | ||
| assert s.error_code == RoborockErrorCode.none | ||
| assert s.error_code_name == "none" | ||
| assert s.state_name == "charging" | ||
| assert s.map_present == 1 | ||
| assert s.in_cleaning == 0 | ||
| assert s.fan_power == 102 | ||
| assert s.water_box_mode == 203 | ||
| assert s.mop_mode == 300 | ||
| assert s.dock_type == RoborockDockTypeCode.empty_wash_fill_dock | ||
| assert s.dock_error_status == RoborockDockErrorCode.ok | ||
| assert s.current_map == 0 |
There was a problem hiding this comment.
Test coverage is incomplete for StatusV2. The dss-derived properties (clear_water_box_status, dirty_water_box_status, dust_bag_status, water_box_filter_status, clean_fluid_status, hatch_door_status, dock_cool_fan_status) are not tested in test_status_v2, unlike the comprehensive testing in test_status for the original Status class.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
This makes status dynamically determined based on the features on the device instead of relying on user provided examples. A PR will follow this one to delete the old status